-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat (client): extract sync API and make DAL optional #1355
Conversation
cc4b21b
to
e5e1b5a
Compare
I would be inclined to support both CLI args and have it default to
I much prefer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work 🔥 ! Left some comments for clarifications
I agree with @samwillis on keeping the --with-dal
to true
by default, although we need to decide if we want to show the deprecation warnings already in that case. When we're happy with making a breaking change with a release we just switch the default.
I'm also a fan of using the Record<TableName, PgType>
, but it does mean that this update would require a regeneration of the client and we still haven't made it clear if that constitutes a breaking change or not.
@@ -191,10 +245,12 @@ export class ElectricClient< | |||
return new ElectricClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not for this PR but I think we should really move to object arguments rather than positional haha
7a2be9b
to
1badd76
Compare
@samwillis @msfstef I addressed most of your comments. The main change is the flip of the flag default. I kept the |
742febf
to
593915a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, ready to merge! I do think we need to confirm that by merging this, although we are keeping the same current behaviour by default, we will be showing deprecation warnings everywhere.
7e612d2
to
692a12a
Compare
…bject instead of a Map.
…to generate an Electric client.
b2f1f0f
to
f2f3bc7
Compare
This PR extracts the sync method out of the DAL by introducing a
subscribe
method on theElectricClient.sync
object. To compute the shape requested by the user we need a description of the database schema, namely:The outgoing relations are the FKs in the table that reference a column in (another) table.
The incoming relations are FKs defined in (another) table that reference a column of this table.
The incoming relations are required in order to be able to traverse FKs in the reverse direction, e.g. this is needed when syncing the following shape because there is no FK from
issues
tocomments
, only fromcomments
toissues
:The database description used to be generated by our generator and included in the generated Electric client. To truly extract the sync API i had to make it independent of the DAL and the generated client. Therefore, i extended the CLI to use the metadata that is provided with the migrations to generate the DB description and bundle that into the app.
Note:
Questions:
electric-sql generate
as by default it will no longer generate the DAL. If you want to generate the DAL, you need to run it with the--with-dal
flag as inelectric-sql generate --with-dal
. Is thi acceptable, or we want to keep generating the DAL by default and have a--no-dal
flag instead?Map<TableName, PgType>
and is now represented as aRecord<TableName, PgType>
. While this is not strictly necessary, this simplifies the generation of the DB description. Downside is that clients generated with the old generator won't work with newer versions of the ts-client and will require re-generating the DAL with the new ts-client. Is this fine or should i change it back to using aMap<TableName, PgType>
?